-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove CheckForOverflowUnderflow #63825
Conversation
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue DetailsRemove set false to CheckForOverflowUnderflow due to it is disabled b…y default. Issue #63725 Please review
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test failure on the 1 item so far looks unrelated to this change.
There are other places in the tree it's set to true as well. Are they needed for some reason? |
I think it is needed in some places possibly due to some overflow or underflow being intended in some places in the runtime? It's just a guess on why it might be true in those spots or if it's set to true just "in case" something like that happens. Either way I think it would need some extensive tests to make sure those are not needed first. |
I see exactly one under runtime tests. It can be trivially replaced by enclosing the body of the test in |
Yeah, not sure where I was searching before when I saw pages of results (both false and true), but we might as well just get rid of it entirely. |
Seems to be a unrelated test (rebasing on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Please let me if I also need to remove CheckForOverflowUnderflow from
|
Yes, please remove it and add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
Remove set false to CheckForOverflowUnderflow due to it is disabled b…y default.
Issue #63725
Please review
Thank you in advance